Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add serializers for bulk license enrollment endpoint #567

Merged
merged 2 commits into from
Jan 25, 2024
Merged

feat: add serializers for bulk license enrollment endpoint #567

merged 2 commits into from
Jan 25, 2024

Conversation

justEhmadSaeed
Copy link
Contributor

@justEhmadSaeed justEhmadSaeed commented Jan 24, 2024 •
edited
Loading

Description

Created serializers for the POST bulk-license-enrollment endpoint to enable query params and request body.

Link to the associated ticket: https://2u-internal.atlassian.net/browse/ENT-8179

Please confirm if any field is out of scope or under the wrong validation.
image

Testing considerations

  • Include instructions for any required manual tests, and any manual testing that has
    already been performed.
  • Include unit and a11y tests as appropriate
  • Consider performance issues.
  • Check that Database migrations are backwards-compatible

Post-review

Squash commits into discrete sets of changes

iloveagent57
iloveagent57 approved these changes Jan 24, 2024
View reviewed changes
license_manager/apps/api/v1/views.py
Comment on lines +1266 to +1271
@extend_schema(
parameters=[
serializers.EnterpriseEnrollmentWithLicenseSubsidyQueryParamsSerializer,
],
request=serializers.EnterpriseEnrollmentWithLicenseSubsidyRequestSerializer,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Consider copoying some of the docs below about expected params into the help_text of your new serializer fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iloveagent57 added the help_text field to serializer fields.

iloveagent57
iloveagent57 reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing you might want to consider is refactoring the validation logic out of _validate_enrollment_request_params() and into the validation of your new serializer classes (could be a follow-up PR).

@justEhmadSaeed justEhmadSaeed force-pushed the justEhmadSaeed/add-serializers-to-bulk-enrollment-api branch from 11a28df to 4ea89b0 Compare January 25, 2024 11:34
@justEhmadSaeed
Copy link
Contributor Author

One other thing you might want to consider is refactoring the validation logic out of _validate_enrollment_request_params() and into the validation of your new serializer classes (could be a follow-up PR).

@iloveagent57 the serializer validators follow a specific format for their errors, therefore, any changes to them must be tested and adjusted in the frontend-app-admin-portal code, where this endpoint is being utilized. That's why, I created a follow-up ticket for that.
Let me know if there are any concerns.

@justEhmadSaeed justEhmadSaeed merged commit dbfd551 into master Jan 25, 2024
7 checks passed
@justEhmadSaeed justEhmadSaeed deleted the justEhmadSaeed/add-serializers-to-bulk-enrollment-api branch January 25, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveagent57 iloveagent57 iloveagent57 approved these changes

@zamanafzal zamanafzal zamanafzal approved these changes

@sameenfatima78 sameenfatima78 sameenfatima78 approved these changes

@hamzawaleed01 hamzawaleed01 Awaiting requested review from hamzawaleed01

@macdiesel macdiesel Awaiting requested review from macdiesel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justEhmadSaeed @iloveagent57 @zamanafzal @sameenfatima78